Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add future=True flag to SA engine #17174

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Dec 12, 2023

Ref #12541

The only fix that was required was a fix to a unit test setup. Here're the details:

Commit instead of flushing in unit test

Rationale:
We use an in-memory sqlite database for quota tests. With SA 2.0 (or with the future flag enabled on the engine), the following conflict happens: (this is a simplified model)

foo = Foo()
session.add(foo)
session.flush()

engine = session.get_bind()
with engine.connect() as conn:
    conn.execute(some-sql)

foo.bar = "new value"
session.commit() # BOOM!!!!  sqlalchemy.orm.exc.StaleDataError: UPDATE statement on table 'galaxy_user' expected to update 1 row(s); 0 were matched.

Reason for BOOM:
With an in-memory database, the underlying dbapi_connection object is the same for the session and the engine.connect(). Here's what happens:

line 10: foo is flushed to the db tmp buffer
line 14: conn is closed on exit from context manager, which issues a rollback, which rolls back whatever is in the tmp buffer - so foo is never inserted.
line 16: foo is updated
line 17: error happens: the session thinks it's updating foo's record in the db, but that record does not exist therefore, "0 rows matched".

Solution: commit instead of flushing - then foo is inserted.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@jdavcs jdavcs added kind/enhancement area/database Galaxy's database or data access layer labels Dec 12, 2023
@jdavcs jdavcs force-pushed the dev_sa20_engine_future branch from 64521bd to 475f722 Compare December 12, 2023 14:54
Rationale:
We use an in-memory sqlite database for quota tests. With SA 2.0 (or
with the `future` flag enabled on the engine), the following conflict
happens: (this is a simplified model)

foo = Foo()
session.add(foo)
session.flush()

engine = session.get_bind()
with engine.connect() as conn:
    conn.execute(some-sql)

foo.bar = "new value"
session.commit() # BOOM!!!!  sqlalchemy.orm.exc.StaleDataError: UPDATE statement on table 'galaxy_user' expected to update 1 row(s); 0 were matched.

Reason for BOOM:
With an in-memory database, the underlying dbapi_connection object is
the same for the session and the engine.connect(). Here's what happens:

line 10: foo is flushed to the db tmp buffer
line 14: conn is closed on exit from context manager, which issues a
rollback, which rolls back whatever is in the tmp buffer - so foo is
never inserted.
line 16: foo is updated
line 17: error happens: the session thinks it's updating foo's record in
the db, but that record does not exist therefore, "0 rows matched".

Solution: commit instead of flushing - then foo is inserted.
@jdavcs jdavcs changed the title [WIP] Add future=True flag to SA engine Add future=True flag to SA engine Dec 12, 2023
@jdavcs jdavcs marked this pull request as ready for review December 12, 2023 19:57
@jdavcs jdavcs requested a review from a team December 12, 2023 19:57
@github-actions github-actions bot added this to the 23.2 milestone Dec 12, 2023
@jdavcs jdavcs mentioned this pull request Dec 12, 2023
4 tasks
@mvdbeek mvdbeek merged commit b922495 into galaxyproject:dev Dec 13, 2023
51 of 52 checks passed
@mvdbeek mvdbeek modified the milestones: 23.2, 24.0 Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/database Galaxy's database or data access layer kind/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants